Would @AndyAyersMS's work with PGO and stack allocation of enumerators handle this automatically rather than needing to manually handle the special-casing?

Would @AndyAyersMS's work with PGO and stack allocation of enumerators handle this automatically rather than needing to manually handle the special-casing?

Seems like it should. If you to do type analysis on a collection to try and leverage a struct enumerator that some specific collection type offers you are more or less duplicating what these (not yet merged) changes do.

There is PR up for the changes (#111473). They are in fairly decent shape, and they optimize the allocation and much of the overhead for abstract enumeration (provided the actual enumerator methods aren't too complicated -- works nicely for Lists, Arrays, etc).

Writeup on the changes in case you want to learn more.

Seems like it should

I lack the expertise to see if that PR would eliminate the enumerator allocation, but I would be in favor of closing this pull request and the associated issue if the JIT is able to handle it.

Seems like it should

I lack the expertise to see if that PR would eliminate the enumerator allocation, but I would be in favor of closing this pull request and the associated issue if the JIT is able to handle it.

Let's hold off on this until Andy's change merges and then validate this isn't actually needed.

Thanks.

It certainly sounds like Andy's change should address this, since List<T> is a specific target in the writeup (https://github.com/dotnet/runtime/blob/41f8397b864dd7280c75d0068b3cb5bfeb55a8a4/docs/design/coreclr/jit/DeabstractionAndConditionalEscapeAnalysis.md#listt).

I love when we are able to make changes to the BCL/JIT/Runtime that means that the simplest code matches (or overtakes) what complex code is striving to do.

There's no particular restriction on which collection types can participate -- the optimization currently keys off of the GetEnumerator call and uses PGO/GDV to specialize for the most likely types seen at runtime. Whether the optimization succeeds on eliminating the enumerator allocation and how well the enumerator gets optimized subsequently depends on the complexity of the enumerator logic, though I will probably be chipping away at removing some of those limitations over time (eg, if the MoveNext has a try we won't optimize).

Behind this there is also a limitation from PGO. It will currently only specialize for the most frequently seen type so if a given use site sees a variety of collections and no one type appears more than about 30% of the time, we won't optimize at all. We have ambitions of broadening this too (multi-guess PGO), but that and conditional escape interact with each other and I haven't yet tried to make the combination work well.

The related change was merged on Feb 4, so the perf analysis should be re-runnable now. Hopefully it says there's no benefit to this anymore 😄

@AndyAyersMS, I just tried this on .NET 10 Preview 4, and it's still showing the List<T>.Enumerator allocation. Is that expected?

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Security.Claims;
using System.Security.Principal;

BenchmarkSwitcher.FromAssembly(typeof(Test).Assembly).Run(args);

[MemoryDiagnoser(false)]
public partial class Test
{
    private ClaimsPrincipal _principal = new(new ClaimsIdentity([new Claim(ClaimTypes.Name, "TestUser")]));

    [Benchmark]
    public IIdentity? Get() => _principal.Identity;
}

@AndyAyersMS, I just tried this on .NET 10 Preview 4, and it's still showing the List<T>.Enumerator allocation. Is that expected?

I'll take a look later today.

@AndyAyersMS, I just tried this on .NET 10 Preview 4, and it's still showing the List<T>.Enumerator allocation. Is that expected?

I'll take a look later today.

We fail to inline the enumerator's MoveNextRare and so can't stack allocate.

The main reason we don't inline is that this method isn't called so the call site is cold. We have other cases like this where inlining itself is not directly beneficial, but it unlocks optimizations elsewhere that are beneficial. Let me see if we can fix this...

Verified that if we tweak inlining the other good things happen (DOTNET_JitInlineAdditionalMultiplier is not available in release). So it's "just" a matter of trying to fix that.

BenchmarkDotNet v0.15.0, Windows 11 (10.0.22631.5335/23H2/2023Update/SunValley3) (Hyper-V)
Intel Xeon Platinum 8370C CPU 2.80GHz (Max: 2.79GHz), 1 CPU, 16 logical and 8 physical cores
.NET SDK 9.0.300
[Host] : .NET 9.0.5 (9.0.525.21509), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI
Job-JCFQHN : .NET 10.0.0 (42.42.42.42424), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI

EnvironmentVariables=DOTNET_JitHashDump=1bff8516,DOTNET_JitStdOutFile=C:\bugs\r111799\jit.d1,DOTNET_JItDumpTier0=0,DOTNET_JitInlineAdditionalMultiplier=2 Toolchain=CoreRun

Verified that if we tweak inlining the other good things happen (DOTNET_JitInlineAdditionalMultiplier is not available in release). So it's "just" a matter of trying to fix that.

#116150

Seemingly the benefit of inlining MoveNextRare is conditional; we'd have to dig into those regressions to see why things don't appear to work out as well for some cases.

The JIT can likely detect when the enumeration is happening via GDV'd interface calls and give an extra inlining boost to cold enumeration methods.